Skip to content

lib: remove redundant destroyHook checks #60120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

lib: remove redundant destroyHook checks #60120

wants to merge 3 commits into from
+8 −19

Conversation

gurgunday
Copy link
Member

// Fewer checks may be possible, but these cover everything.

emitDestroy already checks these now:

node/lib/internal/async_hooks.js

Lines 533 to 538 in 6f941fc

function emitDestroyScript(asyncId) {
// Return early if there are no destroy callbacks, or invalid asyncId.
if (!hasHooks(kDestroy) || asyncId <= 0)
return;
async_wrap.queueDestroyAsyncId(asyncId);
}

We are essentially checking the same condition twice

Also the invalid id check didn't cover undefined. I don't know if it was intentional, but in any case, since #56966, which removed enroll, I'm not sure if we can have an undefined asyncId anyway

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Oct 4, 2025
@codecov Codecov
Copy link

codecov bot commented Oct 4, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.48%. Comparing base (6f941fc) to head (0255b2e).
⚠️ Report is 45 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #60120      +/-   ##
==========================================
+ Coverage   88.45%   88.48%   +0.02%     
==========================================
  Files         703      703              
  Lines      207826   207822       -4     
  Branches    40030    40027       -3     
==========================================
+ Hits       183842   183889      +47     
+ Misses      15973    15940      -33     
+ Partials     8011     7993      -18
Files with missing lines Coverage Δ
lib/internal/async_hooks.js 99.37% <100.00%> (ø)
lib/internal/process/task_queues.js 100.00% <100.00%> (ø)
lib/internal/timers.js 99.72% <100.00%> (+0.13%) ⬆️
lib/timers.js 100.00% <100.00%> (ø)

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Flarna Flarna added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 6, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/69585/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/69588/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@Flarna Flarna Flarna approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gurgunday @nodejs-github-bot @anonrig @Flarna